-
Notifications
You must be signed in to change notification settings - Fork 917
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Workspace] Add workspace list page #6182
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6182 +/- ##
==========================================
+ Coverage 67.42% 67.44% +0.01%
==========================================
Files 3362 3366 +4
Lines 65258 65338 +80
Branches 10524 10541 +17
==========================================
+ Hits 43999 44066 +67
- Misses 18701 18708 +7
- Partials 2558 2564 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo ? model
instead of modal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think modal
may be more appropriate because this is a modal UI component and references the modal component of OUI . What's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works then
generally looking good, need to resolve the conflicts in the CHANGELOG though |
re-running failed checks |
e9beed8
to
63856e7
Compare
63856e7
to
6e68a53
Compare
} | ||
}; | ||
|
||
export const updateWorkspace = ({ application, http }: Core, id: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The function name here is a little bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const updateWorkspace = ({ application, http }: Core, id: string) => { | |
export const navigateToWorkspaceUpdatePage = ({ application, http }: Core, id: string) => { |
if (http && application && returnToHome) { | ||
const homeUrl = application.getUrlForApp('home', { | ||
path: '/', | ||
absolute: false, | ||
}); | ||
const targetUrl = http.basePath.prepend(http.basePath.remove(homeUrl), { | ||
withoutWorkspace: true, | ||
}); | ||
await application.navigateToUrl(targetUrl); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeleteWorkspaceModal should only care about if we delete the workspace successfully and gives a toast, it should be the component who embed the Modal to decide if jump to other page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because both workspace update page and list page use this component and their delete logic is same, so I put this part of the logic in deleteModal for code reuse. There is a param called returnToHome
to decide whether navigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure if we could put delete logic in deleteModal to let it handle directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For update page, it will jump to the homepage when delete successfully.
- For list page, it need to refresh the list page when delete successfully.
The DeleteWorkspaceModal should expose a function props named onDeleteSuccess
so that parent component can decide what to do next. Pass returnToHome
is coupling the DeleteModal and the handle logic of the parent component.
One question: How could a user find this page? From which menu could they navigate to this page? |
There is an entry named |
ada9f5b
to
eabb00d
Compare
Got it, Thanks @raintygao for clarification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
eabb00d
to
d53b2a1
Compare
* feat: add workspace list Signed-off-by: tygao <[email protected]> * doc: update changelog Signed-off-by: tygao <[email protected]> * fix test for delete workspace modal (#299) Signed-off-by: tygao <[email protected]> * update function name and modal Signed-off-by: tygao <[email protected]> --------- Signed-off-by: tygao <[email protected]> (cherry picked from commit 2a94f32) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* feat: add workspace list Signed-off-by: tygao <[email protected]> * doc: update changelog Signed-off-by: tygao <[email protected]> * fix test for delete workspace modal (#299) Signed-off-by: tygao <[email protected]> * update function name and modal Signed-off-by: tygao <[email protected]> --------- Signed-off-by: tygao <[email protected]> (cherry picked from commit 2a94f32) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
In this PR, a new page will be added to workspace module. This page shows all workspaces in a table, user can view the name, id, description and features of each workspace. And users could search workspace through inputing name.
Issues Resolved
#6132
Screenshot
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration